Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http_header_envs option to include http headers as an env var #835

Closed
wants to merge 2 commits into from

Conversation

danlester
Copy link
Contributor

Similar to the QUERY_STRING environment variable that is set on a new kernel instance, this PR adds an HTTP_HEADERS env var which contains specified HTTP headers in a json-encoded dict.

There is a new configuration option, e.g. pass on the command line:

--VoilaConfiguration.http_header_envs="['X-CDSDASHBOARDS-JH-USER','user-agent']"

and then any new Voilà kernel will have an env var called HTTP_HEADERS containing a JSON-encoded dict which includes any of the specified headers that were available. For example:

{'X-CDSDASHBOARDS-JH-USER': '{"kind": "user", "name": "dan", "admin": true}', 'user-agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit...'}

The user-agent string could be used to provide alternative implementations or a warning message for an unsupported browser; the X-CDSDASHBOARDS-JH-USER value can be details of an authenticated user from ContainDS Dashboards (in which case, Voilà should only be accessible via the appropriate JupyterHub proxies, and not directly).

This system augments some really useful existing features or discussions such as #414 and #410.

@nariyar
Copy link

nariyar commented May 22, 2021

Dear @danlester
Is this merge expected to happen anytime soon? This would be a huge help. Will unblock progress of our work as we needed exactly this. Thanks in advance..

@maartenbreddels
Copy link
Member

maartenbreddels commented May 22, 2021 via email

@danlester
Copy link
Contributor Author

@nariyar Thank you for your comment. I'm not a Voilà maintainer so can't merge it myself.

Marten would have to do it (which is why he said he'd take a look).

@maartenbreddels Thanks for taking a look - I'm not aware of any standard that is relevant here to be honest. I guess if you want it to be "similar to HTTP" in some sense, you might want the headers passed on in "Key: Value\n" form (with a blank line at the end...) instead of JSON, but maybe that's a bit weird for env vars really.

I think the most important thing is ease of use on the Voilà (notebook) side - we shouldn't expose Voilà authors to standards just because we think they are "right" from a technical level.

Maybe the easiest thing for the notebook author would be to access headers as individual env vars, e.g. X_HEADER_USER_AGENT and X_HEADER_ANYTHING_ELSE, as long as we can be consistent over naming. That would save the Voilà notebook having to JSON-decode.

@mariobuikhuizen
Copy link
Member

I've looked into the available standards for this (on request of @maartenbreddels) and found that CGI has specifications for HTTP headers: https://datatracker.ietf.org/doc/html/rfc3875#section-4.1.18. As previous changes (#414) to supply server information to the kernel also conformed to the CGI standard, it would make sense to continue applying those standards here too.

Note that HTTP header names are case-insensitive: https://datatracker.ietf.org/doc/html/rfc2616#section-4.2, so the comparison in handler.py should also be done in a case-insensitive way.

Would you have the time and inclination to make these changes @danlester?

@danlester
Copy link
Contributor Author

@mariobuikhuizen Hi there! In theory I would like to help, and will in due course. But not sure when I'll get on to it, so if it is important to anyone else they should please feel free to make a note here that they are working on it :)

@mariobuikhuizen
Copy link
Member

mariobuikhuizen commented Jul 14, 2021

Thank you @danlester, I will start working on it.

@jtpio
Copy link
Member

jtpio commented Sep 2, 2021

Fixed by #922

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants